Skip to content

Update DefBootstrap to handle Error from ClassValue #133604

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 7 commits into from
Aug 27, 2025
Merged

Update DefBootstrap to handle Error from ClassValue #133604

merged 7 commits into from
Aug 27, 2025

Conversation

jdconrad
Copy link
Contributor

ClassValue.getFromHashMap wraps checked exceptions as Error, but we do not want to crash here because we could not process the type correctly as part of a Painless script, so we instead unwrap the Error and rethrow the original exception.

@jdconrad jdconrad requested a review from a team as a code owner August 26, 2025 22:18
@elasticsearchmachine elasticsearchmachine added the Team:Core/Infra Meta label for core/infra team label Aug 26, 2025
@elasticsearchmachine
Copy link
Collaborator

Pinging @elastic/es-core-infra (Team:Core/Infra)

@jdconrad jdconrad added the auto-backport Automatically create backport pull requests when merged label Aug 26, 2025
ldematte
ldematte reviewed Aug 27, 2025
View reviewed changes
Copy link
Contributor

@ldematte ldematte left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would be great (if possible) to add a unit test that forces an Exception (checked) inside ClassValue.getFromHashMap to demonstrate this PR works as intended

@jdconrad jdconrad added >bug and removed >non-issue labels Aug 27, 2025
@elasticsearchmachine
Copy link
Collaborator

Hi @jdconrad, I've created a changelog YAML for you.

@jdconrad
Copy link
Contributor Author

jdconrad commented Aug 27, 2025 •
edited
Loading

@ldematte Thanks for the review! I will try to add a jdk25+ specific test.

Edit: On second thought, did you want something additional to the changes in DefBootstrapTests where this is indeed forced as an error?

ldematte
ldematte approved these changes Aug 27, 2025
View reviewed changes
Copy link
Contributor

@ldematte ldematte left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see your point, the test you have basically already cover what is needed, showing that any exception does indeed get wrapped.
I was thinking about a test showing that when a (checked) exception is thrown in ClassValue.getFromHashMap, that exception is not wrapped anymore but it's instead a PainlessWrappedException.
But IDK how easy or difficult it is to do that, nor if this adds a great value, so I'll let you decide if and how you want to add such a test.

LGTM

@jdconrad jdconrad merged commit 65ec9e8 into elastic:main Aug 27, 2025
33 checks passed
This was referenced Aug 27, 2025
Merged
Merged
jdconrad added a commit to jdconrad/elasticsearch that referenced this pull request Aug 27, 2025
@jdconrad
Update DefBootstrap to handle Error from ClassValue (elastic#133604)
47ab85c 
ClassValue.getFromHashMap wraps checked exceptions as Error, 
but we do not want to crash here because we could not process 
the type correctly as part of a Painless script, so we instead 
unwrap the Error and rethrow the original exception.
@jdconrad jdconrad mentioned this pull request Aug 27, 2025
Merged
jdconrad added a commit to jdconrad/elasticsearch that referenced this pull request Aug 27, 2025
@jdconrad
Update DefBootstrap to handle Error from ClassValue (elastic#133604)
f53df87 
ClassValue.getFromHashMap wraps checked exceptions as Error, 
but we do not want to crash here because we could not process 
the type correctly as part of a Painless script, so we instead 
unwrap the Error and rethrow the original exception.
@jdconrad jdconrad mentioned this pull request Aug 27, 2025
Merged
jdconrad added a commit to jdconrad/elasticsearch that referenced this pull request Aug 27, 2025
@jdconrad
Update DefBootstrap to handle Error from ClassValue (elastic#133604)
514e839 
ClassValue.getFromHashMap wraps checked exceptions as Error, 
but we do not want to crash here because we could not process 
the type correctly as part of a Painless script, so we instead 
unwrap the Error and rethrow the original exception.
@elasticsearchmachine
Copy link
Collaborator

💚 Backport successful

Status Branch Result
✅ 9.1
✅ 9.0
✅ 8.18
✅ 8.19

jdconrad added a commit to jdconrad/elasticsearch that referenced this pull request Aug 27, 2025
@jdconrad
Update DefBootstrap to handle Error from ClassValue (elastic#133604)
242996a 
ClassValue.getFromHashMap wraps checked exceptions as Error, 
but we do not want to crash here because we could not process 
the type correctly as part of a Painless script, so we instead 
unwrap the Error and rethrow the original exception.
elasticsearchmachine pushed a commit that referenced this pull request Aug 27, 2025
@jdconrad
Update DefBootstrap to handle Error from ClassValue (#133604) (#133661)
84061ac 
ClassValue.getFromHashMap wraps checked exceptions as Error, 
but we do not want to crash here because we could not process 
the type correctly as part of a Painless script, so we instead 
unwrap the Error and rethrow the original exception.
elasticsearchmachine pushed a commit that referenced this pull request Aug 27, 2025
@jdconrad
Update DefBootstrap to handle Error from ClassValue (#133604) (#133658)
b2f95d8 
ClassValue.getFromHashMap wraps checked exceptions as Error, 
but we do not want to crash here because we could not process 
the type correctly as part of a Painless script, so we instead 
unwrap the Error and rethrow the original exception.
elasticsearchmachine pushed a commit that referenced this pull request Aug 27, 2025
@jdconrad
Update DefBootstrap to handle Error from ClassValue (#133604) (#133662)
d155df6 
ClassValue.getFromHashMap wraps checked exceptions as Error, 
but we do not want to crash here because we could not process 
the type correctly as part of a Painless script, so we instead 
unwrap the Error and rethrow the original exception.
elasticsearchmachine pushed a commit that referenced this pull request Aug 27, 2025
@jdconrad
Update DefBootstrap to handle Error from ClassValue (#133604) (#133660)
a313641 
ClassValue.getFromHashMap wraps checked exceptions as Error, 
but we do not want to crash here because we could not process 
the type correctly as part of a Painless script, so we instead 
unwrap the Error and rethrow the original exception.
sarog pushed a commit to portsbuild/elasticsearch that referenced this pull request Sep 11, 2025
@jdconrad @sarog
Update DefBootstrap to handle Error from ClassValue (elastic#133604) (e…
5bba30b 
…lastic#133662)

ClassValue.getFromHashMap wraps checked exceptions as Error, 
but we do not want to crash here because we could not process 
the type correctly as part of a Painless script, so we instead 
unwrap the Error and rethrow the original exception.
sarog pushed a commit to portsbuild/elasticsearch that referenced this pull request Sep 19, 2025
@jdconrad @sarog
Update DefBootstrap to handle Error from ClassValue (elastic#133604) (e…
ac1c99d 
…lastic#133662)

ClassValue.getFromHashMap wraps checked exceptions as Error, 
but we do not want to crash here because we could not process 
the type correctly as part of a Painless script, so we instead 
unwrap the Error and rethrow the original exception.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ldematte ldematte ldematte approved these changes

Assignees

No one assigned

Labels

auto-backport Automatically create backport pull requests when merged >bug :Core/Infra/Scripting Scripting abstractions, Painless, and Mustache Team:Core/Infra Meta label for core/infra team v8.18.7 v8.19.4 v9.0.7 v9.1.4 v9.2.0

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@jdconrad @elasticsearchmachine @ldematte